Extend File System Class #215
Conversation
|
@costdev Can you take a look here before I go too far and see if there are any issues with this approach. |
There was a problem hiding this comment.
Thanks for the ping @namithj! I don't see any immediate issues.
I understand the desire to extend WP_Filesystem_Direct as this is indeed extending the underlying functionality, and there's no need to, for example, repeat the contents of WP_Filesystem_Direct::chmod() elsewhere. I think it's fine to keep going.
What other methods do you intend to introduce to this class?
|
I will be overriding the get_contents_array function to introduce a two new parameters (no of lines to read and read direction) which will let us read from bottom up and also modify it to read via file pointer and iterate over the required no of lines instead of pulling the whole content into memory as it does now. |
The Read Routine
PHP Doc Alignment
|
I will try and push the changes today so that we can merge this in |
Code review suggestions
Signed-off-by: Namith Jawahar <[email protected]>
Co-authored-by: Colin Stewart <[email protected]> Signed-off-by: Namith Jawahar <[email protected]>
Add File Exists and readable or writable checks
This method no longer exists.
Previously, new messages were prepended to a log file. They are now appended to the log file. This updates the relevant test to address the new order and contents of the log file.
|
@namithj I'm about to push test updates. There is one expected failure, which requires two source changes in the PR to fix. Once GitHub Actions fails to demonstrate the issue, I'll push the two source changes to verify the fix. |
|
🤔 Seems there's some failures happening with setting up the tests. That's... new. 🤨 |
Subversion was removed from GitHub Actions' default packages for Ubuntu 24. The tests installer uses Subversion internally. This changes installs Subversion.
|
Sweet. There's the expected test failure. Pushing source changes to verify the fix now. |
|
@namithj GitHub Actions passing 🎉 Can you pull down the latest changes in the PR and do some testing in the dashboard to make sure everything still works as you expect it to? |
|
Everything looks good. |
|
I have a few more minor test updates to make for correctness, then I think we'll be ready to merge after that. |
|
@namithj I think that's me done with tests for this one - for now at least. I'm happy to merge this now. What do you think? |
|
Merging |
Extend File System Class
Pull Request
What changed?
(Please list the changes you made...)
Why did it change?
(Please list the reasons you made the changes...)
Did you fix any specific issues?
(Please tag any specific issues here...)
CERTIFICATION
By opening this pull request, I do agree to abide by the Code of Conduct and be bound by the terms of the Contribution Guidelines in effect on the date and time of my contribution as proven by the revision information in GitHub.